🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15526

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 1 Cancelled Job, 4 Unrelated Failures

As of commit dd7c05e with merge base d07a49a ():

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@AdrianLundell Just want to better understand the rationale behind removing per channel support / scratch buffer code and in general stateful ops related code ? (I do understand the FoldDQQ related changes to the pass though), (edited)

The implementation didn't produce numerically results originally so I wanted to fix that before adding per-channel support. Not doing that in this patch was mostly to keep the patch size down and prioritizing the most important functionality before going into details.

Regarding the stateful scratch buffer, since we compute the kernel sum which the buffer was used for AOT and replace the bias, there is no reason to do that in the runtime as I see it. The only use-case I see where that would make sense (using the MVE implementation) is if you are very tight on memory and prefer to spend some extra time during each interference to compute the kernel sum in a scratch buffer rather than keeping it in memory. But then again there is no reason to do everything in one single patch, better to get a good minimal working example running IMO.

The implementation didn't produce numerically results originally so I wanted to fix that before adding per-channel support. Not doing that in this patch was mostly to keep the patch size down and prioritizing the most important functionality before going into details.

Regarding the stateful scratch buffer, since we compute the kernel sum which the buffer was used for AOT and replace the bias, there is no reason to do that in the runtime as I see it. The only use-case I see where that would make sense (using the MVE implementation) is if you are very tight on memory and prefer to spend some extra time during each interference to compute the kernel sum in a scratch buffer rather than keeping it in memory. But then again there is no reason to do everything in one single patch, better to get a good minimal working example running IMO.

• Yes, we need scratch buffer for DSP & MVE (Helium) implementations, I think
• Given that, I would suggest, to keep CMSISScratchBufferContext file as-is , for now. The idea was to evolve this into a generic ScratchBuffer impl that could be reused for couple of diff optimization scenarios
• How are you testing these changes ? E2E on FVP (or) some real device for numerical results. Can you share the steps please ?
• Can we add regression tests specifically for the numerical correctness fixes mentioned ?

Also can you add comments in the code explaining why scratch buffer context is simplified and when it might still be needed ? Overall I think this is a solid refactor in the right direction

* Yes, we need scratch buffer for DSP & MVE (Helium) implementations, I think

We will need scratch buffer for some ops yes, but I am not sure if the header solution for keeping track of the state will ever be needed. As I see it, we can either do the computation AOT as in this case, or it will be reset with every inference as for CONV. Maybe I am missing some use-case here?

* Given that, I would suggest, to keep CMSISScratchBufferContext file as-is , for now. The idea was to evolve this into a generic ScratchBuffer impl that could be reused for couple of diff optimization scenarios

Agree we can keep it for potential future use.

* How are you testing these changes ? E2E on FVP (or) some real device for numerical results.  Can you share the steps please ?

The python op implementation is tested in the test_dialect_linear in backends/cortex_m/test/ops/test_linear.py and the cpp implementation is tested on FVP in the test_implementation_linear tests. The tests for running this after having downloaded executorch are:

python3 -m venv env
source env/bin/activate
pip install -U pip setuptools wheel cmake pytest-cov zstd ninja
./install_executorch.sh --editable
examples/arm/setup.sh --i-agree-to-the-contained-eula
source examples/arm/ethos-u-scratch/setup_path.sh

#To test installation
examples/arm/run.sh

# To build cortex-m test runner
backends/cortex_m/test/build_test_runner.sh

# To run linear tests:
pytest --config-file=backends/arm/test/pytest.ini backends/cortex_m/test/ops/test_linear.py

* Can we add regression tests specifically for the numerical correctness fixes mentioned ?

These are available in test_linear.py. These tests were not passing before the patch.

Also can you add comments in the code explaining why scratch buffer context is simplified and when it might still be needed ? Overall I think this is a solid refactor in the right direction

I think the logic is well explained in the commit message + the quantized_linear_fusion_pass.py

Thanks, looks good over all, approving as we also need to move faster.
Also can you check FVP CI failures (not related ?)

[EDIT:] Looks like there are compilation issues / CI failures (not sure if it is related to #15612)

Fails in XNNPACK/ios-arm64-coreml-fp16 not related